I understand the motivation to make this class generic - it help with testing. However, making this generic makes the code much harder to navigate and understand. We really need to think hard if there's another way to test the code that doesn't necessitate making this class generic.

This is not just a nit to me. Making this class use the concrete peer type, getting rid of all the generic callback functions, and getting rid of the comparator parameter would be a huge improvement in code understandably.

I do know we did something similar in the consensus code. Please think hard if there's another, better solution to the testing problem;

We can eliminate the template all together. The idea is to introduce SquelchHandler abstract class, which declares the squelch/unsquelch handlers. Then implementation in Overlay and UnitTest encapsulate specific object Peer type and implement the handlers. We can provide on a fly handler for UnitTest where we want to have some flexibility. The only disadvantage that I see so far of this approach is that instead of storing Peer in the Slot we are storing SquelchHandler so we have to instantiate this object for every Peer and validator. So the additional memory footprint is nValidators * nPeers. The concept is explained in this code example: https://gist.github.com/gregtatcam/4c2a680ab3e3cd5095f130bfb329e19f

First, thanks for diving into this.

There are a couple of ways we could try to solve this:

1. Templates and callbacks (the current approach). My biggest objection is ability to navigate and understand the code.

2. New base class for Overlay. I haven't coded this up, but I suspect there isn't enough shared implementation between the test and non-test code. So we'll end up testing something very different from the production code.

3. variant based wrapper. I like the concreteness of this, but the test code requires several wrapped types. This solution works best if there was only one (or very few) wrapped types.

4. type erased wrapper (the approach in your patch). This requires an extra allocation, but it is an improvement over the template based approach from a code navigability POV. We may be able to remove the allocation with small object optimization techniques.

5. Write a test framework that can handle the Overlays and Peers. This by far gives the cleanest, most understandable code. But we (1) don't have that framework and (2) unit tests are still valuable.

Given the above, I vote that we go forward with your proposed patch. However, you might want to wait to hear from other reviewers to see if they have other ideas on this issue as well.

We can implement Peer class in unit tests with most of the methods simply returning some value. This is not going to be the test framework or a step towards implementing the test framework but a way to avoid the extra allocation in (4). We can then pass std::weak_ptr<Peer> to all applicable slot methods as we do today with Peer being a concrete type so no template is required. We will have abstract class SquelchHandler with squelch and unsquelch methods. The class is going to be implemented in Overlay and UnitTest. Reference to the base class is going to be passed to Slots and Slot constructors and squelch and unsquelch called as needed. UnitTest can overwrite the handling on a fly providing flexibility of different handlers as needed. Does this work?

Now that this class is simplified to take a single template parameter, consider moving the implementation to a .cpp file and explicitly instantiating the clocks we need for linking.

Wouldn't we have to move ManualClock from the unit test to the Slot's implementation in this case?

We'd have to move the ManualClock to a common header file, and we normally would not include a test file like that in the main code (tho I think we have a good reason in this case). But let's not worry about that now, we can do that later in another PR if we want.

I'd rather not have a test file in the main code. I think a better way is to get rid of the clock template parameter altogether, in another PR.

should be peers

Done

Is "Return the set of peers from which the local node has received the message." accurate?

It is existing behavior, but could you please put a note saying this function will relay valid proposal messages?

Updated

What is the benefit of this function? I see this function is called by OverlayImpl::deletePeer(Peer::id_t const id). I feel it might be easier to understand if deletePeer(id, true); was called directly.

Done

I'd rather not have a test file in the main code. I think a better way is to get rid of the clock template parameter altogether, in another PR.

Please document this parameter in the comment above.

It is documented. This is what it currently states:
@param validator Public Key of the source validator for Validation or Proposal message. Used to check if the message should be squelched.

We shouldn't be logging such malformed messages; rather we should just charge the peer for sending us bad data and we should drop the message on the floor.

Done

Please add a comment that these parameters are experimental.

Done.

Prefer doxygen format:

/** Relay a proposal.

    @param m the serialized proposal
    @param uid the id used to identify this proposal
    @param validator The pubkey of the validator that issued this proposal
    @return the set of peers which have already sent us this proposal
*/

Done

Does this really need to run every second?

Changed to once every 4 seconds. A peer is considered idle if no message is received in 8 seconds.

I'd simply: return addSuppressionPeerWithStatus(key, peer).first;

Done

The HashRouter API was already confusing enough. It's now become a little too confusing. I don't have an improvement to offer, but we need to think about this.